Skip to content

Fix loss of precision for distance between a subseq and itself#860

Merged
seanlaw merged 13 commits into
stumpy-dev:mainfrom
NimaSarajpoor:fix_precision_self_distance
May 8, 2023
Merged

Fix loss of precision for distance between a subseq and itself#860
seanlaw merged 13 commits into
stumpy-dev:mainfrom
NimaSarajpoor:fix_precision_self_distance

Conversation

@NimaSarajpoor

Copy link
Copy Markdown
Collaborator

According to this error, we can realize that there is a loss of precision in self match in core.mass. This can result in loss-of-precision in the output of motifs.

We first add a test function to expose this error.

@NimaSarajpoor NimaSarajpoor changed the title Fix loss of precision for self-match distance Fix loss of precision for distance between a subseq and itself May 6, 2023
@codecov-commenter

codecov-commenter commented May 6, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (8b16995) 99.28% compared to head (1c4b158) 99.28%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #860   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files          83       83           
  Lines       13683    13699   +16     
=======================================
+ Hits        13585    13601   +16     
  Misses         98       98           
Impacted Files Coverage Δ
stumpy/core.py 100.00% <100.00%> (ø)
tests/test_precision.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@NimaSarajpoor

NimaSarajpoor commented May 6, 2023

Copy link
Copy Markdown
Collaborator Author

@seanlaw
Please feel free to review (and merge if there is notthing to change). After the merge, I will resume my work on PR #856.

Comment thread stumpy/core.py
Comment thread stumpy/core.py Outdated
Comment thread stumpy/core.py Outdated
Comment thread stumpy/core.py Outdated
Comment thread stumpy/core.py Outdated
Comment thread stumpy/core.py Outdated
Comment thread tests/test_precision.py
@NimaSarajpoor NimaSarajpoor force-pushed the fix_precision_self_distance branch from 80c051c to 1c4b158 Compare May 8, 2023 02:05
@seanlaw

seanlaw commented May 8, 2023

Copy link
Copy Markdown
Contributor

@NimaSarajpoor It looks like this is ready to be merged. Can you please confirm?

@NimaSarajpoor

Copy link
Copy Markdown
Collaborator Author

I think it is ready.

(FYI: I did not change the added test function. So, the ref value is coming from Naive func as usual.)

@seanlaw seanlaw merged commit 82ce308 into stumpy-dev:main May 8, 2023
@seanlaw

seanlaw commented May 8, 2023

Copy link
Copy Markdown
Contributor

@NimaSarajpoor Thank you for addressing this!

@NimaSarajpoor NimaSarajpoor deleted the fix_precision_self_distance branch September 4, 2023 03:24
seanlaw pushed a commit that referenced this pull request Jul 10, 2026
…1152)

core._find_matches seeds candidate_idx = query_idx and then breaks when
D[candidate_idx] > atol + max_distance, so it relies on D[query_idx] == 0.
core.mass_absolute establishes that, but aamp_match never passed query_idx to
it, unlike motifs.match which passes it to core.mass.

query_idx was added to core.mass and core.mass_absolute in #860; #856 wired it
into core.mass from motifs.match the next day, but aamp_motifs.py was not
updated. core.mass_absolute's query_idx branches have been unreachable since,
which is why they carry `# pragma: no cover`. They are now covered, so the
pragmas are removed.

For a correct self-join the fix is a no-op: cdist returns exactly 0.0 for the
identical row, so D[query_idx] was already the minimum. What it restores is the
"self-match is returned first" guarantee and the "Subsequences `Q` and
`T[query_idx:query_idx+m]` are different" warning when query_idx does not
describe Q -- neither of which could happen on the non-normalized path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants